modified to run captureSession.startRunning() on a background thread #378

hajunho · 2024-02-09T10:04:41Z

It is more robust in thread management, ensuring that the start of the capture session does not block the main thread and that UI updates are performed on the main thread. Additionally, it carefully manages memory by using [weak self] to avoid retained cycles.

AvdLee

Thanks a lot for your contribution!

wetransferplatform · 2024-02-12T08:53:50Z

	Fails
🚫	CIRectangleDetectorTests.testCorrectlyDetectsAndReturnsQuadilateral(): failed - Snapshot does not match reference. @− "/Users/vagrant/git/Tests/WeScanTests/Snapshots/CIRectangleDetectorTests/testCorrectlyDetectsAndReturnsQuadilateral.1.png" @+ "/Users/vagrant/Library/Developer/CoreSimulator/Devices/B7E973E1-E2F0-4254-B50D-223B8780894F/data/tmp/CIRectangleDetectorTests/testCorrectlyDetectsAndReturnsQuadilateral.1.png" To configure output for a custom diff tool, like Kaleidoscope: `SnapshotTesting.diffTool = "ksdiff"` Actual perceptual precision 0.69906247 is less than required 0.97
🚫	VisionRectangleDetectorTests.testCorrectlyDetectsAndReturnsQuadilateral(): failed - Snapshot does not match reference. @− "/Users/vagrant/git/Tests/WeScanTests/Snapshots/VisionRectangleDetectorTests/testCorrectlyDetectsAndReturnsQuadilateral.1.png" @+ "/Users/vagrant/Library/Developer/CoreSimulator/Devices/B7E973E1-E2F0-4254-B50D-223B8780894F/data/tmp/VisionRectangleDetectorTests/testCorrectlyDetectsAndReturnsQuadilateral.1.png" To configure output for a custom diff tool, like Kaleidoscope: `SnapshotTesting.diffTool = "ksdiff"` Newly-taken snapshot does not match reference.

	Warnings
⚠️	'gray' was deprecated in iOS 13.0: renamed to 'UIActivityIndicatorView.Style.medium'
⚠️	'blackTranslucent' was deprecated in iOS 13.0: Use UIBarStyleBlack and set the translucent property to YES instead.
⚠️	'init(source:)' was deprecated in iOS 12.0: Core Image Kernel Language API deprecated. (Define CI_SILENCE_GL_DEPRECATION to silence these warnings)
⚠️	'blackTranslucent' was deprecated in iOS 13.0: Use UIBarStyleBlack and set the translucent property to YES instead.
⚠️	'isAutoStillImageStabilizationEnabled' was deprecated in iOS 13.0
⚠️	'jpegPhotoDataRepresentation(forJPEGSampleBuffer:previewPhotoSampleBuffer:)' was deprecated in iOS 11.0

	Messages
📖	WeScanTests: Executed 60 tests (2 failed, 0 retried, 0 skipped) in 13.227 seconds
📖	Slowest test: RectangleFeaturesFunnelTests/testAddAlternateImage() (4.692s)
📖	Slowest test: RectangleFeaturesFunnelTests/testAddPreviouslyDisplayedRect() (3.144s)
📖	Slowest test: RectangleFeaturesFunnelTests/testAddMinUnderThreshold() (3.025s)
📖	View more details on Bitrise

Code Coverage Report

Name	Coverage
WeScan	37.24%	⚠️

SwiftLint found issues

Severity	File	Reason
Warning	CaptureSessionManager.swift:271	The disabled 'function_parameter_count' rule should be re-enabled before the end of the file (`blanket_disable_command`)
Warning	CaptureSessionManager.swift:147	Use shorthand syntax for optional binding (`shorthand_optional_binding`)
Warning	CaptureSessionManager.swift:154	Use shorthand syntax for optional binding (`shorthand_optional_binding`)
Warning	CaptureSessionManager.swift:165	Use shorthand syntax for optional binding (`shorthand_optional_binding`)

Generated by 🚫 Danger Swift against bbba45c

Sources/WeScan/Scan/CaptureSessionManager.swift

hajunho

I will make an additional pull request reflecting BasThomas's review.

…prevent runtime errors related to unwrapping a nil optional value.

hajunho · 2024-02-14T15:55:08Z

It looks unrelated #375 build issue is affecting Bitrise fail.

hajunho · 2024-02-16T21:48:26Z

Actual perceptual precision 0.69906247 is less than required 0.97

Bitrise succeeded. Please advise, as there isn't a relationship between the threads and the precision of rectangle detection in the current code.

AvdLee · 2024-02-19T09:58:11Z

@hajunho are tests succeeding for you locally? Also, do you agree this test failure seems unrelated to your code changes?

hajunho · 2024-02-20T22:00:37Z

@hajunho are tests succeeding for you locally? Also, do you agree this test failure seems unrelated to your code changes?

Yes, the tests went well locally. The test failures are not related to my code changes. However, errors at runtime should be taken seriously. It seems necessary to look closely at this problem and check if the environment settings or any recent changes have impacted it. Test benches are not easily created. So, I will search the code more and examine the parts needed for performance improvement. Could you provide any additional information or logs related to this test failure or the test bench? That information might help in identifying the cause of the problem. Additionally, I'd like to know if there's a way to visually inspect the two snapshots created by the test bench and then download the test result images for analysis with various tools to identify the differences.

AvdLee · 2024-02-23T12:23:58Z

@hajunho completely agree. I don't think we can access the test snapshots, so we really need to find a way to reproduce the test failure locally. My guess is that it results from a different Simulator used on CI.

CI runs using the following destination:

{ platform:iOS Simulator, id:B7E973E1-E2F0-4254-B50D-223B8780894F, OS:16.4, name:iPhone 14 Pro }

We've also not updated Xcode yet, so it's using Xcode 14.3.1 at this point.

Feel free to take up the challenge to solve the test in this PR! If not, let me know, and we'll merge this PR based on your local CI succession.

hajunho · 2024-02-24T06:09:09Z

@hajunho completely agree. I don't think we can access the test snapshots, so we really need to find a way to reproduce the test failure locally. My guess is that it results from a different Simulator used on CI.

CI runs using the following destination:

{ platform:iOS Simulator, id:B7E973E1-E2F0-4254-B50D-223B8780894F, OS:16.4, name:iPhone 14 Pro }

We've also not updated Xcode yet, so it's using Xcode 14.3.1 at this point.

Feel free to take up the challenge to solve the test in this PR! If not, let me know, and we'll merge this PR based on your local CI succession.

Thank you for your insights. If I'm able to reproduce the issue and find a fix, I'll update the PR with the necessary changes.

hajunho · 2024-02-24T09:07:45Z

@hajunho completely agree. I don't think we can access the test snapshots, so we really need to find a way to reproduce the test failure locally. My guess is that it results from a different Simulator used on CI.
CI runs using the following destination:

{ platform:iOS Simulator, id:B7E973E1-E2F0-4254-B50D-223B8780894F, OS:16.4, name:iPhone 14 Pro }

We've also not updated Xcode yet, so it's using Xcode 14.3.1 at this point.
Feel free to take up the challenge to solve the test in this PR! If not, let me know, and we'll merge this PR based on your local CI succession.

Thank you for your insights. If I'm able to reproduce the issue and find a fix, I'll update the PR with the necessary changes.

The attached image shows the head minus my PR.

In the case of sub-items, there are a lot of fail cases, so it seems like there is confusion about fail for that item. Even if it is not my PR, the test fails in the existing main branch code. The same goes for iPhone SE~15pro. I'll have to spend some time exploring that test case. Can I leave this commit open until then?

hajunho · 2024-02-24T09:24:56Z

@hajunho completely agree. I don't think we can access the test snapshots, so we really need to find a way to reproduce the test failure locally. My guess is that it results from a different Simulator used on CI.
CI runs using the following destination:

{ platform:iOS Simulator, id:B7E973E1-E2F0-4254-B50D-223B8780894F, OS:16.4, name:iPhone 14 Pro }

We've also not updated Xcode yet, so it's using Xcode 14.3.1 at this point.
Feel free to take up the challenge to solve the test in this PR! If not, let me know, and we'll merge this PR based on your local CI succession.

Thank you for your insights. If I'm able to reproduce the issue and find a fix, I'll update the PR with the necessary changes.

The precision numbers in the existing code match ideally, so there is no PR impact. It would be better to separate the test failure into another problem. Please merge when you are ready.

AvdLee · 2024-02-26T08:43:44Z

Thanks for diving deeper into the specifics! 🙏 We'll have a look at tests separately.

hajunho requested a review from a team as a code owner February 9, 2024 10:04

hajunho requested review from BasThomas, AvdLee, raphkoebraam, kairadiagne, armanarutiunov and swaan-miller February 9, 2024 10:04

AvdLee approved these changes Feb 12, 2024

View reviewed changes

BasThomas reviewed Feb 12, 2024

View reviewed changes

Sources/WeScan/Scan/CaptureSessionManager.swift Outdated Show resolved Hide resolved

BasThomas reviewed Feb 12, 2024

View reviewed changes

Sources/WeScan/Scan/CaptureSessionManager.swift Outdated Show resolved Hide resolved

hajunho commented Feb 13, 2024

View reviewed changes

BasThomas approved these changes Feb 13, 2024

View reviewed changes

hajunho force-pushed the master branch from 6550b48 to bbba45c Compare February 14, 2024 14:59

hajunho added 3 commits February 15, 2024 00:25

modified to run captureSession.startRunning() on a background thread

699b747

self is not nil before proceeding in a block of code, which can help …

921741b

…prevent runtime errors related to unwrapping a nil optional value.

Added the missing code in BasThomas's safe code review.

94ed306

hajunho force-pushed the master branch from 6550b48 to 94ed306 Compare February 14, 2024 15:27

AvdLee merged commit cf918f7 into WeTransfer:master Feb 26, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modified to run captureSession.startRunning() on a background thread #378

modified to run captureSession.startRunning() on a background thread #378

hajunho commented Feb 9, 2024

AvdLee left a comment

wetransferplatform commented Feb 12, 2024 •

edited

Loading

hajunho left a comment

hajunho commented Feb 14, 2024

hajunho commented Feb 16, 2024 •

edited

Loading

AvdLee commented Feb 19, 2024

hajunho commented Feb 20, 2024

AvdLee commented Feb 23, 2024

hajunho commented Feb 24, 2024

hajunho commented Feb 24, 2024

hajunho commented Feb 24, 2024

AvdLee commented Feb 26, 2024

modified to run captureSession.startRunning() on a background thread #378

modified to run captureSession.startRunning() on a background thread #378

Conversation

hajunho commented Feb 9, 2024

AvdLee left a comment

Choose a reason for hiding this comment

wetransferplatform commented Feb 12, 2024 • edited Loading

Code Coverage Report

SwiftLint found issues

hajunho left a comment

Choose a reason for hiding this comment

hajunho commented Feb 14, 2024

hajunho commented Feb 16, 2024 • edited Loading

AvdLee commented Feb 19, 2024

hajunho commented Feb 20, 2024

AvdLee commented Feb 23, 2024

hajunho commented Feb 24, 2024

hajunho commented Feb 24, 2024

hajunho commented Feb 24, 2024

AvdLee commented Feb 26, 2024

wetransferplatform commented Feb 12, 2024 •

edited

Loading

hajunho commented Feb 16, 2024 •

edited

Loading